-
Notifications
You must be signed in to change notification settings - Fork 83
fix: addonly EDITS should be handled as COMPLETIONS #2133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…uggestion instead of Edits
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2133 +/- ##
==========================================
+ Coverage 61.24% 61.29% +0.05%
==========================================
Files 261 261
Lines 58291 58504 +213
Branches 3582 3618 +36
==========================================
+ Hits 35699 35860 +161
- Misses 22524 22575 +51
- Partials 68 69 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (line.startsWith('+')) { | ||
| completionSuggestion += line.substring(1) + '\n' | ||
| isInAdditionBlock = true | ||
| } else if (isInAdditionBlock && line.startsWith(' ')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we add this check? Should the addition only contain lines with + at the beginning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i updated this part of code, it should be
if (line.startsWith('+')) {
completionSuggestion += line.substring(1) + '\n'
isInAdditionBlock = true
} else if (isInAdditionBlock && !line.startsWith('+')) {
// End of addition block
isInAdditionBlock = false
}
| * | ||
| **/ | ||
| // TODO: should refine the logic here more, possibly find the first non-empty minus/plus if possible | ||
| if (firstMinusIndex + 1 === firstPlusIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there are insertion on multiple consecutive lines (like - - + +)? Will it be counted as edit or completion?
| } | ||
|
|
||
| if (firstMinusIndex === -1 && firstPlusIndex !== -1) { | ||
| return 'addOnly' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will the + line always be after cursor position?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's parsing the raw unified diff so cursor position here is not relevant. Cursor position will only be critical after we are sure if it's a COMPLETION and should we render it or not.
| const relevantLines = d.linesWithoutHeaders | ||
|
|
||
| if (firstMinusIndex === -1 && firstPlusIndex === -1) { | ||
| return 'edit' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that there are no + and - lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically no, we do this way beause currently all suggestions coming back from Qwen is "EDITS" so we can safely assume they are EDTIS and let clients to decide if we will render it or not, i.e. delegate the resposibility to the 3rd party unidfied diff lib we are using in vscode.
| ): { suggestionContent: string; type: SuggestionType } { | ||
| const diffCategory = categorizeUnifieddiff(unifiedDiff) | ||
| if (diffCategory === 'addOnly') { | ||
| const udiff = readUdiff(unifiedDiff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just call this method one time and pass it to categorizeUnifieddiff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ): { suggestionContent: string; type: SuggestionType } { | ||
| const diffCategory = categorizeUnifieddiff(unifiedDiff) | ||
| if (diffCategory === 'addOnly') { | ||
| const udiff = readUdiff(unifiedDiff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do we use this udiff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep you are right, it's not used here, removed.
| } | ||
| } | ||
|
|
||
| // Backward compatibility, completions will be returned if predictinoType is not specified (either Completion or Edit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, predictionType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| } | ||
|
|
||
| // TODO: current implementation here assumes service only return 1 chunk of edits (consecutive lines) and hacky | ||
| export function extractAdditions(unifiedDiff: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions
The function stops processing when it hits a context line after additions right?
- What happens with separated addition blocks in the same diff?
- Do we handle multi-hunk diffs correctly?
- Should we extract ALL '+' lines regardless of context interruptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- yes it's based on the assumption service doesn't return multi chunk of edits
- no
- i assume this is also for the "multi-hunk" scenarios and no we dont handle it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the very least, we need to add if "multi-hunk" then EDITS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i will take it as follow up as service currently doesn't return such suggestion (multi hunk)
* fix: patch #2133 and handle more variants of FIM suggestions * fix: only consider FIM if the first edit line matches trigger position * fix: test --------- Co-authored-by: atontb <[email protected]>
…
Problem
Solution
related pr aws/aws-toolkit-vscode#7646
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.